-
Notifications
You must be signed in to change notification settings - Fork 59
[ci] Remove /v0/total-amulet-balance and /v0/wallet-balance endpoints #3546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d9b8581 to
291e5fd
Compare
| } | ||
| } | ||
|
|
||
| private def lookupTotalSupplyByLatestRound()(implicit ec: ExecutionContext, tc: TraceContext) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OriolMunoz-da please check if these changes (and further below in this file) are OK, (ignoring noHoldingFeesOnTransfers) otherwise I have to keep getTotalAmuletBalance around in the store, but I would prefer to just delete more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noHoldingFeesOnTransfers.supported is true for all networks now, so you shouldn't need the fallback
the issue is in tests though, because it depends on the latest ACS snapshot which might not be available (as it's computed every 3h)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
apps/app/src/main/scala/org/lfdecentralizedtrust/splice/console/ScanAppReference.scala
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private def lookupTotalSupplyByLatestRound()(implicit ec: ExecutionContext, tc: TraceContext) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noHoldingFeesOnTransfers.supported is true for all networks now, so you shouldn't need the fallback
the issue is in tests though, because it depends on the latest ACS snapshot which might not be available (as it's computed every 3h)
| sv1ScanBackend | ||
| .lookupInstrument("Amulet") | ||
| .flatMap(_.totalSupply) | ||
| .valueOrFail("total balance not yet defined") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example here, how do you know that a snapshot was generated?
(maybe there's a forceSnapshot somewhere that I'm not seeing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always forcing now in getTotalAmuletBalance in ScanAppReference
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm minus the comments Oriol already made, thank you!
I assume we already discussed this with product (probably while I was out), if not let's ofc do that so noone gets surprised by it.
| sql""" | ||
| select greatest( | ||
| 0, | ||
| sum_cumulative_change_to_initial_amount_as_of_round_zero - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also be able to cleanup the dbs and txlog parsers further, perhaps in multiple steps:
- make the columns here and in the scan txlog only required for the balances nullable
- stop writing those fields
- eventually nuke those columns
I'd suggest to move this to separate PRs but let's make sure we push through at least 1 and 2 as it meaningfully simplifies the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add another PR follow up for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #3563 for leftovers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this later though, rather first see what more can be removed and then simply do it all at once.
docs/src/release_notes.rst
Outdated
|
|
||
| - Scan | ||
|
|
||
| - ``/v0/total-amulet-balance`` and ``/v0/wallet-balance`` endpoints have been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest mentioning the replacement endpoints here and also mention that the existing endpoints are already deprecated. "deprecated endpoints have been removed in favor of X" just sounds much nicer than "Y has been removed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
Also removed wallet-balance usage in scan_txlog.py and everything that was using it (removed option |
7e1cd6b to
c483204
Compare
|
FYI I will not merge this until I get OK from Product (we have discussed before, but to be certain) |
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
apps/app/src/main/scala/org/lfdecentralizedtrust/splice/console/ScanAppReference.scala
Outdated
Show resolved
Hide resolved
| ) | ||
| def getTotalAmuletBalance(): Option[BigDecimal] = { | ||
| val _ = forceAcsSnapshotNow() | ||
| lookupInstrument("Amulet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to make this an error instead of an optional? it seems like we really always expect amulet to be present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah can do that, it's only used for testing now anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ) | ||
| def getTotalAmuletBalance(): Option[BigDecimal] = { | ||
| val _ = forceAcsSnapshotNow() | ||
| lookupInstrument("Amulet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: read this from whatever config we have for this instead of hardcoding the string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok done
| "../token-standard/dependencies/canton-json-api-v2/openapi-ts-client": { | ||
| "name": "@lfdecentralizedtrust/canton-json-api-v2", | ||
| "version": "3.4.0-SNAPSHOT", | ||
| "version": "3.4.8-SNAPSHOT", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand why this changed now but it does seem correct 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't know, I did a npmFix and npmLint at some point..
a9745e9 to
b0ad3c4
Compare
OriolMunoz-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
1e57f3a to
d4e02d9
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 90b485e6f06c872792e27907b008f730a2e4e2ce in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/47691 |
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
…e/ScanAppReference.scala Co-authored-by: moritzkiefer-da <[email protected]> Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
Signed-off-by: Raymond Roestenburg <[email protected]>
90b485e to
0eb5150
Compare
Fixes #3503
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines